Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: make temp path customizable #3325

Merged
merged 2 commits into from
Dec 30, 2015

Conversation

jbergstroem
Copy link
Member

In CI we previously passed NODE_COMMON_PIPE to the test runner to avoid long filenames on Windows. Avoid long paths by changing the temporary directory instead. This also allows us to run test suites in parallel since NODE_COMMON_PIPE otherwise would have been used from multiple tests.

/R=@nodejs/build, @Trott ?

@jbergstroem
Copy link
Member Author

CI (without passing NODE_TMP_DIR): https://ci.nodejs.org/job/node-test-commit/804/

@jbergstroem jbergstroem added the build Issues and PRs related to build files or the CI. label Oct 12, 2015
@@ -6,6 +6,8 @@ var assert = require('assert');
var os = require('os');
var child_process = require('child_process');

const testRoot = process.env.NODE_TEST_DIR || path.dirname(__filename);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__dirname perhaps?

@rvagg
Copy link
Member

rvagg commented Oct 12, 2015

I'm still catching up but did NODE_COMMON_PIPE get removed from all the tests already?

@jbergstroem
Copy link
Member Author

AFAIK it was never in tests, just passed to exports.PIPE [edit: if passed to common.js].

@mscdex mscdex added test Issues and PRs related to the tests. and removed build Issues and PRs related to build files or the CI. labels Oct 12, 2015
@Trott
Copy link
Member

Trott commented Oct 12, 2015

I'll leave it to someone more in tune with the entirety of the build process to LGTM or not, but I'm +1. The common.PIPE improvements that build on this to enable parallel testing will be in a subsequent PR?

@jbergstroem
Copy link
Member Author

Prior to removal, if NODE_COMMON_PIPE was unset exports.PIPE would be stored in the temporary directory which works fine in parallel runs since the thread id is appended to common.tmpDir. The change I'm introducing is the ability to change where tmpDir lives.

@jasnell
Copy link
Member

jasnell commented Oct 22, 2015

@jbergstroem ... what's the status on this? Looks like it needs to be rebased at a minimum

@jbergstroem
Copy link
Member Author

@jasnell waiting for feedback/review. I'll rebase.

@jbergstroem
Copy link
Member Author

@Trott to answer your question regarding "enabling" parallel builds -- that would be something we'd most likely be changing in the jenkins setup.

@joaocgreis
Copy link
Member

LGTM

Can you ensure that testRoot exists? Since this is now a variable, it can be useful to set it to a directory that does not exist yet.

To run parallel tests on Windows, the pipe name should also depend on the thread number. What about adding something like lines 64 and 66 after 139?

@jbergstroem
Copy link
Member Author

@joaocgreis does the pipe really have to depend on a thread number since it already lives in its own thread folder (tmp.$thread)?

Regarding creating the directory, that's already part of refreshTmpDir.

@joaocgreis
Copy link
Member

@jbergstroem the pipe is defined differently in windows, it is a global name, not a file in the test folder (as created in line 139).

The tmp.$thread directories are created by refreshTmpDir, but testRoot is not. Currently this is not a problem since the test directory always exists, but if you set NODE_TEST_DIR to a directory that does not exit, tests fail. There could be a mkdir(testRoot) or just a descriptive exception if it does not exist, but on the other hand the error is pretty clear so feel free to ignore this suggestion.

@jbergstroem
Copy link
Member Author

Rebased and fixed an issue with two tests that assumed a specific folder structure. CI (which again shouldn't branch into this): https://ci.nodejs.org/job/node-test-commit/1324/

@jbergstroem
Copy link
Member Author

Oh, and @joaocgreis -- I now create the dir through refreshTmp.

@@ -64,18 +66,19 @@ function rmdirSync(p, originalEr) {
}

exports.refreshTmpDir = function() {
if (!fs.existsSync(testRoot)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

existsSync() is deprecated so maybe use one of the suggested alternatives?

@jbergstroem
Copy link
Member Author

ping!

@@ -155,21 +162,13 @@ Object.defineProperty(exports, 'hasFipsCrypto', {

if (exports.isWindows) {
exports.PIPE = '\\\\.\\pipe\\libuv-test';
if (process.env.TEST_THREAD_ID) {
exports.PIPE = +'.' + process.env.TEST_THREAD_ID;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs space between + and '.'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be +=

@joaocgreis
Copy link
Member

@jbergstroem wouldn't checking the NODE_TEST_DIR directly in the test runner be better?

@jbergstroem
Copy link
Member Author

@joaocgreis checking in the runner now. Also, created an option called --test-dir supplementing NODE_TEST_DIR.

@jbergstroem
Copy link
Member Author

CI (should be unaffected): https://ci.nodejs.org/job/node-test-commit/1560/

jasnell pushed a commit that referenced this pull request Jan 15, 2016
In CI we previously passed `NODE_COMMON_PIPE` to the test runner to
avoid long filenames. Add an option to the test runner that allows the
user to change the temporary directory instead. This also allows us to
run test suites in parallel since `NODE_COMMON_PIPE` otherwise would
have been used from multiple tests at the same time.

PR-URL: #3325
Reviewed-By: Joao Reis <reis@janeasystems.com>
jasnell pushed a commit that referenced this pull request Jan 15, 2016
A few tests assumed that temp dirs always lived in the same
parent folder as fixtures. Make these use `common.tmpDir` instead.

PR-URL: #3325
Reviewed-By: Joao Reis <reis@janeasystems.com>
@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

Landed in v4.x-staging in d33279d and b3079ef

MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
In CI we previously passed `NODE_COMMON_PIPE` to the test runner to
avoid long filenames. Add an option to the test runner that allows the
user to change the temporary directory instead. This also allows us to
run test suites in parallel since `NODE_COMMON_PIPE` otherwise would
have been used from multiple tests at the same time.

PR-URL: #3325
Reviewed-By: Joao Reis <reis@janeasystems.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
A few tests assumed that temp dirs always lived in the same
parent folder as fixtures. Make these use `common.tmpDir` instead.

PR-URL: #3325
Reviewed-By: Joao Reis <reis@janeasystems.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
In CI we previously passed `NODE_COMMON_PIPE` to the test runner to
avoid long filenames. Add an option to the test runner that allows the
user to change the temporary directory instead. This also allows us to
run test suites in parallel since `NODE_COMMON_PIPE` otherwise would
have been used from multiple tests at the same time.

PR-URL: nodejs#3325
Reviewed-By: Joao Reis <reis@janeasystems.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
A few tests assumed that temp dirs always lived in the same
parent folder as fixtures. Make these use `common.tmpDir` instead.

PR-URL: nodejs#3325
Reviewed-By: Joao Reis <reis@janeasystems.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants